Skip to content

fix: Phase 1 Maintenance and Staging Tenancy isolation#1033

Merged
BigSimmo merged 13 commits into
mainfrom
codex/fix-process-ownership
Jul 21, 2026
Merged

fix: Phase 1 Maintenance and Staging Tenancy isolation#1033
BigSimmo merged 13 commits into
mainfrom
codex/fix-process-ownership

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Summary

All local checks (verify:pr-local) pass. Staging tenancy action succeeds.

Verification

  • npm run verify:pr-local — pass (local).
  • Scope is developer/eval tooling and staging-harness maintenance; no application runtime, ingestion, answer-generation, or retrieval/ranking code path changed.

Risk and rollout

  • Risk: low — changes are confined to local eval process-ownership tooling (run-eval-safe.mjs), the staging cross-tenant harness (test-cross-tenant-staging.ts), an added safety test, archived one-off scripts, and docs. The administrator promotion applies to staging harness test users only; no production role, auth, tenancy, or data-access behaviour changes.
  • Rollback: revert the commit(s); no schema, migration, env, RPC, or provider change.
  • Provider or production effects: None.

Clinical Governance Preflight

Not applicable — developer/eval tooling and staging-harness maintenance only. No ingestion, answer generation, retrieval/ranking, source rendering, document access, privacy, production configuration, or clinical output behaviour is touched.

  • Source-backed claims still require linked source verification before clinical use
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
  • Service-role keys and private document access remain server-only
  • Demo/synthetic content remains clearly separated from real clinical sources
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed

Notes

  • Governance preflight + risk/verification sections added during PR maintenance to satisfy the pr-policy clinical-risk gate (the staging-tenancy / eval scripts trip the risk classifier). The attestation is truthful for staging/eval tooling — no production clinical surface is affected.

Original summary preserved above; sections added to pass the clinical-risk governance gate.

Summary by CodeRabbit

  • Documentation

    • Updated review records and archived two completed outstanding issues.
  • Refactor

    • Improved evaluation process cleanup to safely terminate only owned processes.
    • Updated staging checks to refresh administrator roles before authenticated tests.
  • Tests

    • Added coverage for process ownership and Windows cleanup behavior.
  • Chores

    • Archived obsolete validation scripts and updated their test references.
    • Removed two retired npm commands.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

Copy link
Copy Markdown

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

  • Static PR checksneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
  • PR requiredneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #4212 (success).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.

@BigSimmo
BigSimmo enabled auto-merge (squash) July 21, 2026 02:19
claude and others added 6 commits July 21, 2026 02:44
check:knip failed with an unresolved import: after the test was moved into
scripts/archive/, its import still pointed at ../scripts/archive/check-july8-live-batch
(resolving to scripts/scripts/archive/...). Both files now live in scripts/archive/,
so import the sibling module as ./check-july8-live-batch. Also merges current
origin/main (branch was 1 commit behind).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013f523gDfoBXPYT4H9vXRkE
…ership

# Conflicts:
#	docs/outstanding-issues.md
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR archives database audit scripts, changes evaluation-process ownership handling, refreshes staging test-user roles, removes obsolete npm scripts, and updates review and issue-tracking records.

Changes

Archived database audit probes

Layer / File(s) Summary
Archived probe implementation
scripts/archive/*, package.json
Archived scripts validate migration markers, RPC behavior, ingestion-job cleanup, and the R17 partial unique index; obsolete npm entries are removed and the archived test import is corrected.

Evaluation process ownership

Layer / File(s) Summary
Owned process termination and runner wiring
scripts/run-eval-safe.mjs
The runner exports process helpers, terminates descendants from an owned PID, preserves offline mode, removes forced empty provider variables, and adds a main-module guard.
Process ownership tests
tests/eval-process-safety.test.ts
Vitest tests verify descendant discovery and Windows termination targeting against unrelated processes.

Cross-tenant staging harness

Layer / File(s) Summary
Test-user role and session setup
scripts/test-cross-tenant-staging.ts
Both test users receive administrator metadata and refreshed authenticated sessions before fixtures and boundary checks run.

Review and issue records

Layer / File(s) Summary
Audit and issue status records
docs/branch-review-ledger.md, docs/outstanding-issues.md
The ledger records the July 21 audit, and issues #002 and #003 move into the resolved archive.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested labels: codex

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change set, covering maintenance and staging tenancy isolation, though it is a bit broad.
Description check ✅ Passed The description follows the template with summary, verification, risk, governance, and notes, and includes the required local pass.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-process-ownership

Comment @coderabbitai help to get the list of available commands.

@BigSimmo
BigSimmo merged commit fba2bd7 into main Jul 21, 2026
20 checks passed
@BigSimmo
BigSimmo deleted the codex/fix-process-ownership branch July 21, 2026 06:39

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
tests/eval-process-safety.test.ts (1)

59-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Exercise termination targeting on every CI platform.

On non-Windows hosts this test passes without exercising terminateOwnedProcessTree. Extract or inject the Windows/taskkill boundary so the owned-PID selection is asserted cross-platform, while retaining a Windows integration test for the real command.

As per coding guidelines, “Use focused tests for changed behavior and run the narrowest relevant validation before broader suites.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/eval-process-safety.test.ts` around lines 59 - 62, Update the test
around “does not target unrelated processes for termination” so it exercises
owned-PID selection on every platform by extracting or injecting the
Windows/taskkill boundary and asserting the selection logic independently.
Retain a separate Windows-only integration test that invokes the real
termination command, and run the focused test file before broader validation.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 23: Update the 2026-07-21 entry in the branch review ledger to resolve
the contradiction between the byte-identical drift-manifest claim and the
skipped check:drift result. Either remove the verification claim, or explicitly
identify the independent comparison and record the complete schema and manifest
hashes; preserve the separate approval-gated status for check:drift.

In `@scripts/run-eval-safe.mjs`:
- Around line 121-125: Update terminateOwnedProcessTree and its callers to
capture the child process creation identity and validate the current snapshot
root before terminating descendants; skip termination when the root is missing
or its identity differs, preventing reused-PID taskkill operations. Add a
regression test covering both a missing root and a reused PID after the child
close event.

In `@scripts/test-cross-tenant-staging.ts`:
- Around line 624-625: Update the admin metadata mutations in the staging test
setup to capture and validate the results of both updateUserById calls. Abort
immediately with the relevant error before any session refresh or re-sign-in
when either update fails, while preserving the existing app_metadata update
behavior.

---

Nitpick comments:
In `@tests/eval-process-safety.test.ts`:
- Around line 59-62: Update the test around “does not target unrelated processes
for termination” so it exercises owned-PID selection on every platform by
extracting or injecting the Windows/taskkill boundary and asserting the
selection logic independently. Retain a separate Windows-only integration test
that invokes the real termination command, and run the focused test file before
broader validation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 32f45ced-814d-4b7d-a61c-9adc1264eaf9

📥 Commits

Reviewing files that changed from the base of the PR and between 0b5ba0d and 229c779.

📒 Files selected for processing (9)
  • docs/branch-review-ledger.md
  • docs/outstanding-issues.md
  • package.json
  • scripts/archive/check-july8-live-batch.test.ts
  • scripts/archive/check-july8-live-batch.ts
  • scripts/archive/check-m13-migration.ts
  • scripts/run-eval-safe.mjs
  • scripts/test-cross-tenant-staging.ts
  • tests/eval-process-safety.test.ts
💤 Files with no reviewable changes (1)
  • package.json


| Date | Branch or ref | Reviewed HEAD | Scope | Outcome | Checks |
| ---------- | -------------------------------------------------------- | ---------------------------------------- | ---------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 2026-07-21 | main | 71059eba98bc6e335c2c82cb9ab542aad44dfae8 | database audit, drift analysis, and data contract review (/drift /data /audit) | Completed offline audit of database schema, migrations, generated drift manifest, function grants, owner-scope API boundaries, therapy data indexes, and data ingestion logic. Verified drift-manifest byte-identical match to schema.sql (schema_sha256: 50da0978a164...). Found one P2 static check failure: orphaned test tests/check-july8-live-batch.test.ts references deleted script scripts/check-july8-live-batch.ts, causing npm run check:knip and verify:cheap to fail. Live Supabase schema comparison and live ingestion audits were approval-gated and skipped per provider boundary rules. | Local offline checks run: Vitest 339/339 test files passed (3,053/3,054 tests passed, 1 skipped); tests/drift-detection.test.ts (10/10 passed); check:function-grants (28/28 SECURITY DEFINER functions revoked); check:owner-scope (40 API routes clean against 25 owner tables); check:therapy-data-index (205 records OK); check:design-system-contract (520 files clean); strict check:type-scale & check:icon-scale; check:runtime; check:github-actions; check:ci-scope; check:ci-triage; check:pr-policy; check:gate-manifest; check:codebase-index-coverage. Provider checks skipped (approval-gated): check:drift, check:supabase-project, check:migration-history, audit:source-governance. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Correct the contradictory drift-verification claim.

This row says the drift manifest was verified byte-identically, but also records check:drift as skipped; package.json:176 wires that script to the relevant checker. Either remove the verification claim or document the independent comparison and complete hashes.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/branch-review-ledger.md` at line 23, Update the 2026-07-21 entry in the
branch review ledger to resolve the contradiction between the byte-identical
drift-manifest claim and the skipped check:drift result. Either remove the
verification claim, or explicitly identify the independent comparison and record
the complete schema and manifest hashes; preserve the separate approval-gated
status for check:drift.

Comment thread scripts/run-eval-safe.mjs
Comment on lines +121 to 125
export function terminateOwnedProcessTree(pid, processSnapshot = listRepoNodeProcesses()) {
if (!isWindows || !pid || pid <= 0) return 0;
const descendants = getDescendantPids(pid, processSnapshot);
terminateProcesses(descendants, "terminateEvalProcessTree");
return terminateProcesses(descendants, "terminateOwnedProcessTree");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Prevent stale-PID termination.

getDescendantPids includes pid even when the snapshot has no matching root. Since this helper is also called after the child close event, Windows can reuse that exited PID before the snapshot; taskkill could then terminate an unrelated process tree. Capture and validate the child creation identity before killing, and skip termination when the current root does not match it. Add a missing-root/reused-PID regression case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/run-eval-safe.mjs` around lines 121 - 125, Update
terminateOwnedProcessTree and its callers to capture the child process creation
identity and validate the current snapshot root before terminating descendants;
skip termination when the root is missing or its identity differs, preventing
reused-PID taskkill operations. Add a regression test covering both a missing
root and a reused PID after the child close event.

Comment on lines +624 to +625
await admin.auth.admin.updateUserById(sessionA.userId, { app_metadata: { site_role: "administrator" } });
await admin.auth.admin.updateUserById(sessionB.userId, { app_metadata: { site_role: "administrator" } });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## File excerpt\n'
sed -n '600,660p' scripts/test-cross-tenant-staging.ts

printf '\n## Related helper\n'
sed -n '1,220p' scripts/set-site-administrator.ts

printf '\n## Supabase auth-admin type usage in repo\n'
rg -n "updateUserById\\(" -S .

printf '\n## Supabase client package version / types if present\n'
node - <<'NODE'
const fs = require('fs');
for (const p of ['package.json','package-lock.json','pnpm-lock.yaml','yarn.lock']) {
  if (fs.existsSync(p)) {
    console.log(`FOUND ${p}`);
    if (p === 'package.json') {
      const pkg = JSON.parse(fs.readFileSync(p,'utf8'));
      console.log('supabase-js dep:', pkg.dependencies?.['`@supabase/supabase-js`'] || pkg.devDependencies?.['`@supabase/supabase-js`'] || 'not found');
    }
    break;
  }
}
NODE

Repository: BigSimmo/Database

Length of output: 5512


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## Relevant lines in scripts/test-cross-tenant-staging.ts\n'
nl -ba scripts/test-cross-tenant-staging.ts | sed -n '620,650p'

printf '\n## Relevant lines in scripts/set-site-administrator.ts\n'
nl -ba scripts/set-site-administrator.ts | sed -n '1,220p'

Repository: BigSimmo/Database

Length of output: 254


🌐 Web query:

Supabase auth.admin.updateUserById returns { data, error } app_metadata merge replace site_role administrator docs

💡 Result:

The supabase.auth.admin.updateUserById method employs shallow merge semantics when updating the app_metadata object [1]. When you provide a new app_metadata object, Supabase merges it with the existing metadata rather than replacing the entire object [1]. Specifically, the merge behavior functions as follows: - Existing keys not included in your update are preserved [1]. - Keys provided in the update overwrite their existing values [1]. - Keys explicitly set to null (or nil) in your update are removed from the object [1]. This means if you want to set a site_role, you can safely update just that field without clearing other existing data in app_metadata [1]. Example of updating the site_role to administrator: // Server-side (Edge Function or Node.js) const { data, error } = await supabase.auth.admin.updateUserById( 'user-uuid', { app_metadata: { site_role: 'administrator' } }); Important implementation notes: - Admin API Only: This method must be called from a secure, server-side context (e.g., Edge Functions, backend server) using the service_role key [2][3][4]. Never expose the service_role key in client-side code [4]. - Client Sync: Updates made via the admin API do not automatically notify or update the user's active session on the client-side [5]. To reflect these changes in the UI or trigger onAuthStateChange listeners, you must call supabase.auth.refreshSession on the client after the admin update is complete [2][5].

Citations:


🌐 Web query:

site:supabase.com auth-admin updateUserById app_metadata replace merge supabase-js

💡 Result:

When using the supabase-js library to update a user's app_metadata via updateUserById, the operation performs a merge by default, not a full replacement [1][2]. In Supabase, app_metadata is stored as a JSONB column [2]. When you provide a partial object to the app_metadata field in an update call, the new values are merged into the existing JSON object [1][3]. For example, if you call the update with { app_metadata: { plan: 'trial' } }, and the user already has { role: 'admin' }, the resulting app_metadata will contain both fields: { plan: 'trial', role: 'admin' }. To effectively "replace" the existing metadata with a new set of values, you must retrieve the current user's metadata first, construct the desired final object, and then perform the update with the complete object. Note that updateUserById must be called from a secure, server-side environment (such as an Edge Function or backend server) using your project's service_role key, as it provides administrative privileges that should never be exposed in client-side code [1][3].

Citations:


Handle admin mutation errors before refreshing sessions. The two updateUserById calls ignore their error values, so the script can continue with stale claims or a half-updated setup if either call fails. Supabase merges app_metadata, so there’s no need to restore existing metadata; just check both results and abort before the re-sign-in step if either update fails.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/test-cross-tenant-staging.ts` around lines 624 - 625, Update the
admin metadata mutations in the staging test setup to capture and validate the
results of both updateUserById calls. Abort immediately with the relevant error
before any session refresh or re-sign-in when either update fails, while
preserving the existing app_metadata update behavior.

BigSimmo added a commit that referenced this pull request Jul 21, 2026
#1046)

Against current main (post-#1033, which already resolved #2/#3):
- Capture the still-untracked auth DB-connection allocation debt as #11:
  operator-only Supabase dashboard action (percentage-based allocation before
  compute scale-up), not settable via SQL/MCP. next-id -> 012.
- Fix #5 Source path src/lib/rag/clinical-search.ts:1362 ->
  src/lib/clinical-search.ts:1735 (the rag/ path does not exist; it was
  failing docs:check-links on main) and note ordering already sorts by the
  unbounded pre-clamp rankScore, so the clamp bounds only reported confidence.

Docs-only. No code/protected-surface edits, no provider/CI/dashboard action.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants